Reuse the same ID for both auth-less and auth-ful INVITEs #488
Conversation
| inviteOKRetryAttemptsNoACK = 2 | ||
| inviteOkAckLateTimeout = inviteOkRetryIntervalMax | ||
|
|
||
| inviteCredentialValidity = 60 * time.Minute // Allow reuse of credentials for 1h |
There was a problem hiding this comment.
Not sure this is smart, might want to extend this to max_call_duration or something.
Also, keep in mind that this is per Call-ID for now, so new calls would still need re-auth.
| tr := callTransportFromReq(req) | ||
| legTr := legTransportFromReq(req) | ||
| log := s.log.WithValues( | ||
| "callID", callID, |
There was a problem hiding this comment.
The one log line that will not have callID now is "Bad request", when validation fails.
alexlivekit
changed the title
| if err != nil { | ||
| return nil, fmt.Errorf("invalid challenge %q: %w", challengeStr, err) | ||
| } | ||
| toHeader := resp.To() |
There was a problem hiding this comment.
Now that we're doing the right validation on the inbound side, the outbound side E2E test caught this error!
But this also means out clients might run into the same issue, in case some of them are not spec-compliant.
pkg/sip/inbound.go
Outdated
| s.inProgressInvites[key] = is | ||
|
|
||
| go func() { | ||
| time.Sleep(inviteCredentialValidity) |
There was a problem hiding this comment.
Better avoid spawning goroutines that wait for the whole hour without a clear cancellation signal.
Usually, you'd have one goroutine periodically cleaning the expired cache. time.AfterFunc could work, but again, there's no real need to create thousands of timers all waiting for an hour.
pkg/sip/inbound.go
Outdated
| log: log, | ||
| s: s, | ||
| id: id, | ||
| id: "unassigned", |
There was a problem hiding this comment.
Would be helpful if it logs/panics if something tries to read this unassigned ID.
There was a problem hiding this comment.
I would rather not add shims or verification for this. Unless it's real clean, I'd rather leave it blank. Would that be better in your opinion?
alexlivekit
changed the title
alexlivekit
force-pushed
the
reuse-callid-auth
branch
from
0658b5d to
de0faf0
Compare
| toTag: toTag, | ||
| fromTag: fromTag, | ||
| } | ||
| s.imu.Lock() |
There was a problem hiding this comment.
A common practice to avoid lock contention is to use RWMutex and doing a two stage lock:
s.imu.RLock()
is, ok := s.inProgressInvites[key]
s.imu.RUnlock()
if ok {
return is
}
s.imu.Lock()
defer s.imu.Unlock()
is, ok := s.inProgressInvites[key]
if ok {
return is
}
// ... the rest ...This allows multiple readers to get the invite state without blocking each other. Also notice that we redo the check after getting a write lock - other routine might create the state earlier.
There was a problem hiding this comment.
This pattern:
- Adds a few lines of code - maintenance complexity over time.
- Adds a few operations - Every first invite will have to go through two lock/unlock cycles (userspace, but still not 0us).
- Optimization? - Does it save time in this case? Sipgo de-dupes retransmissions of all requests, and in-dialog INVITEs are not possible before we conclude the initial invite. So the only possible race condition here is two separate re-INVITE requests that are not retransmissions. In that case, the flow is
lock->check exists->unlock->return existingwith or without this pattern. What am I missing here? - Consistency - If we just always do it that way, that make a solid practice and approach.
My initial approach was "keep it simple", I'll happily change that for consistency's sake, but I would love to make sure I understand the performance aspect of this.
See this thread for more details. Main point:
When we authenticate for the purposes of a SIP session (when credentials are not being cached), the following happens:
If we happen to count the 407 response as a failed call, and we generate unique SCL_* ids for both.